Skip to content

feat(agent-file-preview): show previews in files overlay#16713

Merged
0xfullex merged 20 commits into
mainfrom
feat/file-preview-overlay
Jul 6, 2026
Merged

feat(agent-file-preview): show previews in files overlay#16713
0xfullex merged 20 commits into
mainfrom
feat/file-preview-overlay

Conversation

@AtomsH4

@AtomsH4 AtomsH4 commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

🚨 Branch strategy — read before opening this PR

The v2 refactor has merged into main, so main is the default branch for active development (v1 and v2 code currently coexist there — expect large, breaking changes).

  • Active development (features, refactors, optimizations, fixes for the current codebase) → target main (the default base).
  • v1 maintenance (hotfixes and subsequent v1 releases) → branch from and target v1, not main.

A v1 fix does not auto-carry to main: if the same bug exists on main, open a separate forward-port PR targeting main. Before touching subsystems being replaced, read docs/references/data/ and watch for @deprecated markers — they flag code being deleted.

What this PR does

Before this PR:
Agent Chat opened artifact file links in a separate file preview tab. Selecting a file could also turn the Files tab into a file-specific preview surface, so the right pane no longer behaved like a stable file browser.

After this PR:
Agent Chat keeps a single stable Files tab. File links and file-tree selections open a focused, dismissible preview overlay inside the Files panel; the overlay can be closed with the close button or Escape, and stale selections are cleared when the selected file disappears.

The Files panel now keeps the file tree as the underlying surface, supports external absolute-path previews, refreshes the tree and expanded lazy directories from the search row, and re-reads source-viewable previews on refresh. The file tree search row includes refresh and workspace external-open controls, and row context menus expose default app, reveal-in-folder, and detected editor actions.

20260703174517_rec_

Fixes # N/A

Why we need it and why it was done in this way

The following tradeoffs were made:

  • Reworked ArtifactPaneView around a single file-tree surface with an overlay preview instead of a separate preview tab, so Agent Chat tabs stay stable.
  • Removed the artifact-pane file-tree splitter and its persisted width key because the preview now covers the tree instead of sitting beside it; resizing remains at the right-pane level.
  • Kept refresh scoped to the file-tree model and expanded directories while also refreshing source-viewable preview content. PDF previews keep their existing layout refresh path, and Office previews restart through their existing refresh key.
  • Reused the existing external-app detection and file APIs for file-tree actions instead of adding a new opener path.

The following alternatives were considered:

  • Keeping the separate file-preview tab, but that makes the Files tab unstable and splits file browsing from previewing.
  • Keeping a side-by-side resizable tree/preview layout, but the overlay behavior no longer needs a second pane width or persisted tree-width preference.
  • Using a modal or global drawer, but file previews should stay scoped to the Agent Files panel.

Links to places where the discussion took place: N/A

Breaking changes

None.

If this PR introduces breaking changes, please describe the changes and the impact on users.

Special notes for your reviewer

Current GitHub checks for this PR show basic-checks, render-test, catalog, changes, and changeset checks passing; general-test and notify are skipped by the workflow.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • Branch: This PR targets the correct branch — main for active development, v1 for v1 maintenance fixes
  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
  • Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

Agent Chat file previews now open as a dismissible overlay inside the Files panel, with refresh and open-with actions available from the file tree.

Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
@AtomsH4 AtomsH4 requested review from a team and Copilot July 3, 2026 09:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Agent Chat “Files” experience so file selections open in an in-panel preview overlay (keeping the Files tab stable), while also enriching the file tree search row and context menus with refresh/external-open actions.

Changes:

  • Switch Agent Chat artifact file previews from a separate “file-preview” tab to an overlay preview within the Files panel.
  • Extend ArtifactPaneView/FileTree to support overlay preview controls, search-row toolbar content, and contextual external-open actions.
  • Adjust right pane resize-handle stacking and update/expand related renderer tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/renderer/pages/agents/components/AgentRightPane/AgentRightPane.tsx Moves artifact file preview into the Files tab via ArtifactPaneView overlay mode and updates selection/open/close behavior.
src/renderer/pages/agents/tests/AgentChatArtifactPane.test.tsx Updates agent chat artifact-pane tests to assert overlay-based previews and close behavior.
src/renderer/components/FileTree/types.ts Adds searchToolbar and searchClearLabel props to support richer search-row UI.
src/renderer/components/FileTree/FileTree.tsx Renders optional search-row toolbar and allows customizable clear-button aria-label.
src/renderer/components/FileTree/tests/FileTree.test.tsx Adds coverage for search toolbar rendering and clear-button usability.
src/renderer/components/chat/shell/RightPaneHost.tsx Raises resize-handle z-index to ensure it stays above overlay content.
src/renderer/components/chat/shell/tests/RightPaneHost.test.tsx Adds a regression test for resize-handle stacking above overlays.
src/renderer/components/chat/panes/OpenExternalAppButton.tsx Refines split-button styling via consistent grouped classes.
src/renderer/components/chat/panes/tests/OpenExternalAppButton.test.tsx Updates mocks/assertions to match the updated split-button group behavior.
src/renderer/components/chat/panes/ArtifactPane.tsx Adds overlay preview mode, preview selection plumbing, search-row toolbar, and external-open context menu actions.
src/renderer/components/chat/panes/tests/ArtifactPane.test.tsx Adds tests for overlay preview open/close, Escape handling, refresh behavior, and context menu external-open actions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 280 to 284
useEffect(() => {
if (!selectedFile || !fileTreeModel.hasLoaded) return
if (isSelectableFileNode(fileTreeModel.nodeById, selectedFile)) return
if (previewFileSelection?.workspacePath === workspacePath && previewFileSelection?.filePath === selectedFile) return
setSelectedFile(null)

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was translated automatically.

Findings:

  • Important: src/renderer/components/chat/panes/ArtifactPane.tsx:779 only handles Escape when the key event originates inside the overlay subtree. In the normal path, the user clicks a file-tree row to open the preview, so focus remains on that row behind the absolute overlay; because the overlay is tabIndex={-1} but is never focused, pressing Escape does not bubble through handleOverlayKeyDown and the keyboard close path does not work. Please either focus the overlay when it opens or attach the Escape handler at a container/window level while the overlay is active, and add a regression test that presses Escape after opening from the file row.

Summary:
The overlay preview flow is otherwise well-covered, and I did not repeat Copilot's existing stale-selection comment. This Escape focus-path issue is user-visible but has a close-button fallback, so I am leaving this as a comment review rather than blocking the PR.

Verification:

  • Reviewed PR #16713 at head 2d8cce53e1f01c6a64f3bd4a079e9b23e17ff72f in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260703-16713-2d8cce53 with three reviewer slices plus coordinator pass.
  • Checked GitHub CI for the reviewed head: changes, catalog-hand-edit-check, changeset-check, basic-checks, and render-test passed; general-test, notify, and rerun-ci skipped.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository review rules.

After fixing, please request eeee0717 for further review.


Original Content

Findings:

  • Important: src/renderer/components/chat/panes/ArtifactPane.tsx:779 only handles Escape when the key event originates inside the overlay subtree. In the normal path, the user clicks a file-tree row to open the preview, so focus remains on that row behind the absolute overlay; because the overlay is tabIndex={-1} but is never focused, pressing Escape does not bubble through handleOverlayKeyDown and the keyboard close path does not work. Please either focus the overlay when it opens or attach the Escape handler at a container/window level while the overlay is active, and add a regression test that presses Escape after opening from the file row.

Summary:
The overlay preview flow is otherwise well-covered, and I did not repeat Copilot's existing stale-selection comment. This Escape focus-path issue is user-visible but has a close-button fallback, so I am leaving this as a comment review rather than blocking the PR.

Verification:

  • Reviewed PR #16713 at head 2d8cce53e1f01c6a64f3bd4a079e9b23e17ff72f in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260703-16713-2d8cce53 with three reviewer slices plus coordinator pass.
  • Checked GitHub CI for the reviewed head: changes, catalog-hand-edit-check, changeset-check, basic-checks, and render-test passed; general-test, notify, and rerun-ci skipped.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository review rules.

修复后请 request eeee0717 再进行后续 review。

@ousugo ousugo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and submitted one confirmed issue for this PR: native context-menu mode may not trigger Agent file-tree menu on new actions.

searchPlaceholder={t('agent.preview_pane.search_placeholder')}
searchToolbar={searchToolbar}
searchClearLabel={t('common.clear')}
getMenuItems={getFileTreeMenuItems}

@ousugo ousugo Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review comment was translated automatically.

[B1] In the scenario where getMenuItems adds ArtifactPane, the right-click menu is wrapped as CommandContextMenu. When the user sets native menu mode, onContextMenu={(e) => e.stopPropagation()} on FileTreeRow blocks event bubbling to CommandContextMenu's outer onContextMenu, causing the menu not to appear. It is recommended not to unconditionally block right-click event bubbling, or to bind the native handler directly to the row element to avoid breaking the native menu mode.


Original Content

[B1]getMenuItems 新增到 ArtifactPane 的场景下,右键菜单被包装为 CommandContextMenu。当用户设为 native 菜单模式时,FileTreeRow 上的 onContextMenu={(e) => e.stopPropagation()} 会阻断事件冒泡到 CommandContextMenu 的外层 onContextMenu,导致该菜单不弹出。建议不要无条件阻断右键事件冒泡,或将 native handler 改为直接绑定到 row 元素上,避免与原生菜单模式断链。

@AtomsH4 AtomsH4 Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review comment was translated automatically.

Fixed: Removed unconditional onContextMenu.stopPropagation() on FileTreeRow, contextmenu events can now bubble up to the outer CommandContextMenu wrapper; added regression test allows row context-menu events to reach the menu wrapper. Corresponding commit: 1da9eae.


Original Content

已修复:移除了 FileTreeRow 上无条件的 onContextMenu.stopPropagation(),contextmenu 事件现在可以冒泡到外层 CommandContextMenu wrapper;新增了 allows row context-menu events to reach the menu wrapper 回归测试。对应 commit: 1da9eae

@kangfenmao kangfenmao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was translated automatically.

There are still a few issues with the current head that need to be fixed before merging:

  • New inline: After fileTreeOpen defaults to true, the AgentRightPane provider will start the workspace directory tree/watcher even when the right panel is closed; this causes unnecessary scanning and watching for large workspaces, and bypasses the intent of filesEnabled=false.
  • Existing unresolved: After deleting a file, selection/overlay may still retain stale preview.
  • Existing unresolved: In native menu mode, stopPropagation() in FileTreeRow prevents the newly added context menu from receiving events.
  • Existing unresolved: After overlay opens, it doesn't get focus, so the Escape close path is unavailable when opening preview via normal file tree click.

I refreshed and confirmed the head is still 2d8cce53e1f01c6a64f3bd4a079e9b23e17ff72f, core CI is still pass/skip combination; per repo review rules, I didn't run pnpm lint/test/format locally.


Original Content

还有几个当前 head 仍然存在的问题,需要修完后再合并:

  • 新增 inline:fileTreeOpen 默认变成 true 后,AgentRightPane provider 在右侧面板关闭时也会启动 workspace directory tree / watcher;大 workspace 下会产生不必要的扫描和监听,也绕过了 filesEnabled=false 的意图。
  • 现有 unresolved:删除文件后 selection/overlay 仍可能保留 stale preview。
  • 现有 unresolved:native 菜单模式下,FileTreeRow 的 stopPropagation() 会让新增的右键菜单接不到事件。
  • 现有 unresolved:overlay 打开后未获得焦点,Escape 关闭路径在正常点击文件树打开预览时不可用。

我刷新确认过 head 仍是 2d8cce53e1f01c6a64f3bd4a079e9b23e17ff72f,核心 CI 仍为通过/跳过组合;按 repo review 规则没有本地跑 pnpm lint/test/format

const [previewFileSelection, setPreviewFileSelection] = useState<ArtifactPaneFileSelection | null>(null)
const [selectedFile, setSelectedFile] = useState<string | null>(null)
const [fileTreeOpen, setFileTreeOpen] = useState(false)
const [fileTreeOpen, setFileTreeOpen] = useState(true)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[B1] Gate the file-tree model by pane visibility

This now initializes fileTreeOpen to true and passes it straight into useArtifactFileTreeModel, whose useWorkspaceFileTree(treeOpen ? workspacePath : undefined) creates the directory tree/watcher. The AgentRightPane provider stays mounted even when Shell is closed, and RightPaneHost open={state.open} only hides the child surface, so simply entering an Agent session with a workspace will start scanning/watching the tree before the user opens the Files pane. This can be expensive on large workspaces, and it also bypasses filesEnabled=false branches because the model is built before the tab is rendered. Please gate this with filesEnabled plus the shell's open/visible state, or keep the tree collapsed until the user opens Files or opens a preview.

@AtomsH4 AtomsH4 Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review comment was translated automatically.

Fixed: treeOpen in useArtifactFileTreeModel is now computed as filesEnabled !== false && fileTreeOpen && shellState.open. The workspace tree/watcher will not start when the right panel is closed or filesEnabled=false. Also added two regression tests for closing the panel and files disabled. Corresponding commit: 46089c1.


Original Content

已修复:useArtifactFileTreeModeltreeOpen 现在由 filesEnabled !== false && fileTreeOpen && shellState.open 计算,右侧面板关闭或 filesEnabled=false 时不会启动 workspace tree/watcher;也补了关闭面板和 files disabled 两个回归测试。对应 commit: 46089c1

AtomsH4 added 6 commits July 4, 2026 19:51
Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
@AtomsH4

AtomsH4 commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

This comment was translated automatically.

@eeee0717 @kangfenmao I've verified against the latest PR head 29ca171c191119431e6717e3960ae53385756825, and the issues mentioned in your human review comments have been fixed:

  • file tree watcher gating: useArtifactFileTreeModel's treeOpen is now controlled by filesEnabled, fileTreeOpen, and shellState.open simultaneously. The watcher won't start when the right panel is closed or files are disabled; added corresponding tests. Commit: 46089c1.
  • stale preview after deletion: When a selected file disappears from the loaded tree, previewFileSelection and selectedFile will be cleared to avoid keeping the old overlay after deletion; added regression tests. Commit: 2685558.
  • native context menu bubbling: FileTreeRow has removed the unconditional contextmenu.stopPropagation(), so right-click events can bubble up to the outer CommandContextMenu; added regression tests. Commit: 1da9eae.
  • Escape focus path: After the overlay opens, it will focus on the overlay. After normally clicking to open preview from file tree, you can directly press Escape to close it; added regression tests. Commit: 18565f8.

I haven't resolved the review threads, leaving them for the reviewers to re-check.


Original Content

@eeee0717 @kangfenmao 已按最新 PR head 29ca171c191119431e6717e3960ae53385756825 核对,你们 human review comments 里提到的问题已修复:

  • file tree watcher gating:useArtifactFileTreeModeltreeOpen 现在同时受 filesEnabledfileTreeOpenshellState.open 控制,右侧面板关闭或 files disabled 时不会启动 watcher;补了对应测试。Commit: 46089c1
  • stale preview after deletion:当已选文件从已加载 tree 中消失时会清空 previewFileSelectionselectedFile,避免删除后保留旧 overlay;补了回归测试。Commit: 2685558
  • native context menu bubbling:FileTreeRow 已移除无条件 contextmenu.stopPropagation(),右键事件可冒泡到外层 CommandContextMenu;补了回归测试。Commit: 1da9eae
  • Escape focus path:overlay 打开后会 focus 到 overlay,正常从 file tree 点击打开预览后可直接按 Escape 关闭;补了回归测试。Commit: 18565f8

我没有 resolve review threads,留给 reviewer 复核。

@AtomsH4 AtomsH4 requested review from eeee0717, kangfenmao and ousugo July 4, 2026 14:24

@ousugo ousugo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was translated automatically.

After reviewing the latest head 29ca171c191119431e6717e3960ae53385756825, I confirm:

  • stale selection / stale overlay fixed: when files disappear from tree model, matching preview and selectedFile are cleaned up, and regression tests were added.
  • overlay Escape close path fixed: overlay actively focuses after opening, pressing Escape after opening normally from file tree can close it, and tests were added.
  • native context menu mode fixed: removed unconditional stopPropagation for contextmenu in FileTreeRow, events can bubble to CommandContextMenu wrapper, and tests were added.
  • file-tree watcher gating fixed: tree model is now controlled by filesEnabled !== false && fileTreeOpen && shellState.open, watcher won't start when right panel is closed or files disabled, and tests were added.

I did not find any new confirmed issues.


Original Content

复核最新 head 29ca171c191119431e6717e3960ae53385756825 后确认:

  • stale selection / stale overlay 已修复:文件从 tree model 消失时会清理 matching preview 与 selectedFile,并补了回归测试。
  • overlay Escape 关闭路径已修复:overlay 打开后会主动 focus,正常从文件树点击打开后按 Escape 可关闭,并补了测试。
  • native context menu 模式已修复:移除了 FileTreeRow 对 contextmenu 的无条件 stopPropagation,事件能冒泡到 CommandContextMenu wrapper,并补了测试。
  • file-tree watcher gating 已修复:tree model 现在受 filesEnabled !== false && fileTreeOpen && shellState.open 控制,右侧面板关闭或 files disabled 时不会启动 watcher,并补了测试。

我没有发现新的确认问题。

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was translated automatically.

Findings:

  • Important: src/renderer/components/chat/panes/ArtifactPane.tsx:1115 only clears selectedFile when the standalone ArtifactPane overlay mode discovers that the loaded tree no longer contains the selected file. A file-tree selection in this standalone path also populated internalPreviewFileSelection, and ArtifactPaneView prefers that value when deriving overlaySelection, so the overlay can keep rendering a deleted file after the tree selection has been cleared. The AgentRightPane path now clears both states, but this newly exposed standalone overlay contract still needs the same cleanup or a regression test covering it.

Summary:
The previously reported AgentRightPane issues are fixed at this head: watcher gating, stale overlay cleanup in the app path, context-menu bubbling, and Escape focus behavior all have corresponding coverage. I found one non-blocking cleanup gap in the standalone ArtifactPane overlay API, so I am leaving this as a comment review rather than requesting changes.

Verification:

  • Reviewed PR #16713 at head 97a260a0d274109dff5e272a1bb6841cec50d630 in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260704T150345Z-16713-97a260a with three reviewer slices plus coordinator pass.
  • Checked GitHub CI for the reviewed head: changes, catalog-hand-edit-check, changeset-check, basic-checks, render-test, and translate passed; general-test and notify skipped.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository review rules.

After the fix, please request eeee0717 for further review.


Original Content

Findings:

  • Important: src/renderer/components/chat/panes/ArtifactPane.tsx:1115 only clears selectedFile when the standalone ArtifactPane overlay mode discovers that the loaded tree no longer contains the selected file. A file-tree selection in this standalone path also populated internalPreviewFileSelection, and ArtifactPaneView prefers that value when deriving overlaySelection, so the overlay can keep rendering a deleted file after the tree selection has been cleared. The AgentRightPane path now clears both states, but this newly exposed standalone overlay contract still needs the same cleanup or a regression test covering it.

Summary:
The previously reported AgentRightPane issues are fixed at this head: watcher gating, stale overlay cleanup in the app path, context-menu bubbling, and Escape focus behavior all have corresponding coverage. I found one non-blocking cleanup gap in the standalone ArtifactPane overlay API, so I am leaving this as a comment review rather than requesting changes.

Verification:

  • Reviewed PR #16713 at head 97a260a0d274109dff5e272a1bb6841cec50d630 in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260704T150345Z-16713-97a260a with three reviewer slices plus coordinator pass.
  • Checked GitHub CI for the reviewed head: changes, catalog-hand-edit-check, changeset-check, basic-checks, render-test, and translate passed; general-test and notify skipped.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository review rules.

修复后请 request eeee0717 再进行后续 review。

Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
@AtomsH4

AtomsH4 commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

This comment was translated automatically.

@eeee0717 Fixed the stale preview gap in standalone ArtifactPane overlay.

  • Invalid selection cleanup now uniformly goes through setSelectedFile(null), which in uncontrolled overlay mode will clear both internalPreviewFileSelection and selectedFile.
  • Added regression test: after standalone overlay opens README.md from file tree, push watcher removed event to confirm both overlay and markdown preview are cleared.
  • Adjusted overlay refresh test to only cover "refreshing tree doesn't re-read preview content when current file still exists"; delete/remove scenarios are covered separately by the new test.

Verification:

  • pnpm exec vitest run --project renderer src/renderer/components/chat/panes/__tests__/ArtifactPane.test.tsx -t "clears the standalone preview overlay" (fails first, passes after fix)
  • pnpm exec vitest run --project renderer src/renderer/components/chat/panes/__tests__/ArtifactPane.test.tsx (72/72 passed)
  • pnpm test (1237 files, 14454 tests passed, 65 skipped)
  • pnpm typecheck
  • pnpm i18n:check

Full pnpm lint fails locally due to pollution from git-ignored .claude/worktrees and .local/corepack directories; targeted eslint on changed files has no errors, and pre-commit staged Biome/ESLint also passes.


Original Content

@eeee0717 已修复 standalone ArtifactPane overlay 的 stale preview gap。

  • 失效 selection cleanup 现在统一走 setSelectedFile(null),uncontrolled overlay mode 下会同时清掉 internalPreviewFileSelectionselectedFile
  • 新增回归测试:standalone overlay 从 file tree 打开 README.md 后推送 watcher removed 事件,确认 overlay 和 markdown preview 都会清掉。
  • 调整 overlay refresh 测试,让它只覆盖"当前文件仍存在时刷新 tree 不重读 preview content",删除/移除场景由新测试单独覆盖。

Verification:

  • pnpm exec vitest run --project renderer src/renderer/components/chat/panes/__tests__/ArtifactPane.test.tsx -t "clears the standalone preview overlay"(先失败,修复后通过)
  • pnpm exec vitest run --project renderer src/renderer/components/chat/panes/__tests__/ArtifactPane.test.tsx(72/72 passed)
  • pnpm test(1237 files, 14454 tests passed, 65 skipped)
  • pnpm typecheck
  • pnpm i18n:check

Full pnpm lint 在本地被 git-ignored 的 .claude/worktrees.local/corepack 目录污染而失败;改动文件的 targeted eslint 无 error,pre-commit staged Biome/ESLint 也通过。

@AtomsH4 AtomsH4 requested a review from eeee0717 July 5, 2026 02:25

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was translated automatically.

Findings:

  • Important: src/renderer/pages/agents/components/AgentRightPane/AgentRightPane.tsx:203 still starts the workspace file-tree model whenever the right pane is open and fileTreeOpen is true, regardless of the active tab. Because this PR also changes fileTreeOpen to default to true, opening the Status or Trace tab directly now materializes the workspace tree/watcher even when the Files tab is not being viewed. That preserves a variant of the previous large-workspace scanning problem for non-Files right-pane usage. Please include the active tab in this gate, e.g. only enable the tree model when activeTab === 'files' in addition to filesEnabled !== false, fileTreeOpen, and shellState.open.

Summary:
The earlier issues around standalone overlay stale-selection cleanup, Escape focus, native context-menu bubbling, and closed-pane/files-disabled watcher gating are fixed at this head. This remaining active-tab gate is still user-visible for large workspaces, so I am requesting changes.

Verification:

  • Reviewed PR #16713 at head 24af41cf99d1f0fe85968cea49489769766f7fba in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260705T0300Z-16713-24af41c with three reviewer slices plus coordinator pass.
  • Checked GitHub CI for the reviewed head: changes, catalog-hand-edit-check, changeset-check, basic-checks, and render-test passed; general-test and notify skipped.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository review rules.

After fixing, please request eeee0717 for further review.


Original Content

Findings:

  • Important: src/renderer/pages/agents/components/AgentRightPane/AgentRightPane.tsx:203 still starts the workspace file-tree model whenever the right pane is open and fileTreeOpen is true, regardless of the active tab. Because this PR also changes fileTreeOpen to default to true, opening the Status or Trace tab directly now materializes the workspace tree/watcher even when the Files tab is not being viewed. That preserves a variant of the previous large-workspace scanning problem for non-Files right-pane usage. Please include the active tab in this gate, e.g. only enable the tree model when activeTab === 'files' in addition to filesEnabled !== false, fileTreeOpen, and shellState.open.

Summary:
The earlier issues around standalone overlay stale-selection cleanup, Escape focus, native context-menu bubbling, and closed-pane/files-disabled watcher gating are fixed at this head. This remaining active-tab gate is still user-visible for large workspaces, so I am requesting changes.

Verification:

  • Reviewed PR #16713 at head 24af41cf99d1f0fe85968cea49489769766f7fba in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260705T0300Z-16713-24af41c with three reviewer slices plus coordinator pass.
  • Checked GitHub CI for the reviewed head: changes, catalog-hand-edit-check, changeset-check, basic-checks, and render-test passed; general-test and notify skipped.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository review rules.

修复后请 request eeee0717 再进行后续 review。

@AtomsH4 AtomsH4 requested a review from eeee0717 July 5, 2026 16:38
Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
@AtomsH4 AtomsH4 force-pushed the feat/file-preview-overlay branch from 6867880 to d693f62 Compare July 5, 2026 17:00
Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
@AtomsH4 AtomsH4 requested a review from 0xfullex as a code owner July 5, 2026 19:39
@AtomsH4

AtomsH4 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

This comment was translated automatically.

@eeee0717 Fixed the active-tab gate based on the latest feedback.

  • The file-tree model gate in AgentRightPane is now filesEnabled !== false && shellState.open && activeTab === 'files', so when opening non-Files tabs like Status / Trace, it won't materialize the workspace tree / watcher.
  • Added regression test keeps the file-tree model closed when opening a non-files tab, covering the path where the right panel is open but the current tab is not Files.
  • Current PR head: 988773a2584fbfee6d7005af315b78f3c903d925; corresponding fix commit: d693f62915.

Verification:

  • pnpm exec vitest run --project renderer src/renderer/pages/agents/components/AgentRightPane/__tests__/AgentRightPane.test.tsx (7/7 passed)
  • GitHub CI for current head: changes, catalog-hand-edit-check, changeset-check, basic-checks, render-test passed; general-test, notify, rerun-ci skipped.

I haven't resolved the review threads, the review request is still there, please re-review.


Original Content

@eeee0717 已按最新 feedback 修复 active-tab gate。

  • AgentRightPane 里的 file-tree model gate 现在是 filesEnabled !== false && shellState.open && activeTab === 'files',打开 Status / Trace 等非 Files tab 时不会 materialize workspace tree / watcher。
  • 新增回归测试 keeps the file-tree model closed when opening a non-files tab,覆盖右侧面板打开但当前 tab 不是 Files 的路径。
  • 当前 PR head: 988773a2584fbfee6d7005af315b78f3c903d925;对应修复提交: d693f62915

Verification:

  • pnpm exec vitest run --project renderer src/renderer/pages/agents/components/AgentRightPane/__tests__/AgentRightPane.test.tsx(7/7 passed)
  • 当前 head 的 GitHub CI:changescatalog-hand-edit-checkchangeset-checkbasic-checksrender-test passed;general-testnotifyrerun-ci skipped。

我没有 resolve review threads,review request 还在,麻烦复核。

@kangfenmao kangfenmao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One functional regression in a shared component. Otherwise the overlay refactor looks solid and the prior review comments appear addressed.

data-kind={node.kind}
onClick={handleRowClick}
onContextMenu={(e) => e.stopPropagation()}
title={node.name}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[B7/B3] Removing the unconditional onContextMenu stopPropagation regresses NotesSidebar's context menu

This deletes onContextMenu={(e) => e.stopPropagation()} from the shared FileTreeRow to fix the native-menu case in ArtifactPane (which has no outer menu). But FileTreeRow is also used by NotesSidebar (NotesSidebar.tsx:375), where the whole FileTree is wrapped in an outer CommandContextMenu (the non-empty "New note / New folder / Upload…" empty-area menu) while each row also has its own CommandContextMenu.

In the default cherry presentation mode, CommandContextMenu uses Radix ContextMenuTrigger, whose onContextMenu calls handleOpen + preventDefault but not stopPropagation. The row's stopPropagation was what kept a row right-click from also bubbling to the outer empty-area trigger. With it removed, right-clicking a note row now opens both the row menu and the empty-area menu at once (the empty-area one ends up on top), so users can no longer reliably reach the row actions.

Native mode is unaffected because handleNativeContextMenu calls stopPropagation itself — only the default cherry mode regresses. The added test (allows row context-menu events to reach the menu wrapper) only asserts the bubbling that causes this; it doesn't cover the nested-wrapper case.

Suggested direction (author's call): fix it upstream so nested cherry-mode menus don't double-open — e.g. have CommandContextMenu's cherry-mode trigger stop propagation of the contextmenu event once it handles it — or reintroduce a scoped stop on the row that still lets the row's own wrapper receive the event.

@AtomsH4 AtomsH4 Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review comment was translated automatically.

Verified current head 74d64c35dc5d1f2dbbde63836deecad0dcfe6e56, this nested cherry-mode context menu issue has been fixed.

The fix is in 81eb5d48d5: CommandContextMenu's cherry-mode trigger now calls event.stopPropagation() after handling contextmenu, so the inner row menu won't bubble up to the NotesSidebar outer empty-area menu after handling right-click; native mode still uses stopPropagation() in handleNativeContextMenu.

Also added a regression test stops cherry context-menu events after an inner menu handles them, covering that the outer onOpenChange doesn't trigger when the inner menu opens. The latest 74d64c35dc is just documentation updates, not affecting this behavior.


Original Content

已核对当前 head 74d64c35dc5d1f2dbbde63836deecad0dcfe6e56,这个 nested cherry-mode context menu 问题已经修复。

修复在 81eb5d48d5CommandContextMenu 的 cherry-mode trigger 现在在处理 contextmenu 后调用 event.stopPropagation(),所以内层 row menu 处理右键后不会再冒泡到 NotesSidebar 外层 empty-area menu;native mode 仍沿用 handleNativeContextMenu 里的 stopPropagation()

也补了回归测试 stops cherry context-menu events after an inner menu handles them,覆盖内层 menu 打开时外层 onOpenChange 不触发。最新的 74d64c35dc 只是文档更新,没有影响这部分行为。

Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>

@zhangjiadi225 zhangjiadi225 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C5] v2-refactor-temp/docs/chat/chat-layout-modes.md:145 still says dynamic Agent tabs include file-preview, but this PR removes the separate preview tab and keeps file previews as an overlay inside the stable Files tab. Please update that note to remove file-preview or describe the Files overlay behavior, so the architecture doc matches the new right-pane contract.

I did not repeat the existing FileTree nested context-menu thread; latest head 81eb5d48d5 appears to address it with the CommandContextMenu bubbling fix and a regression test.

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings:

  • No new code findings in this pass. The previously reported overlay and watcher issues appear addressed at head 81eb5d48d5f03e79eae961b5ccf2a7e32b1f3c8c: file-tree model gating now includes the active Files tab, stale overlay cleanup is covered, Escape focus handling is covered, and the nested context-menu propagation regression appears fixed.

Open questions:

  • None.

Summary:
No additional code issues found. I am not duplicating the existing architecture-doc comment, but v2-refactor-temp/docs/chat/chat-layout-modes.md still has the noted stale file-preview wording and should be handled before this is treated as fully clean.

Verification:

  • Reviewed PR #16713 at head 81eb5d48d5f03e79eae961b5ccf2a7e32b1f3c8c in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260706T0700Z/pr-16713-81eb5d4 with three reviewer passes.
  • Checked GitHub CI: changes, catalog-hand-edit-check, changeset-check, basic-checks, and render-test passed; general-test and notify skipped.
  • Ran git diff --check in reviewer passes.
  • Did not run local pnpm lint/test/format, per repository PR review rules.

Signed-off-by: gujiaming <52187003+AtomsH4@users.noreply.github.com>
@AtomsH4

AtomsH4 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

This comment was translated automatically.

@zhangjiadi225 Updated v2-refactor-temp/docs/chat/chat-layout-modes.md, removed the outdated file-preview dynamic tab description, and changed it to explain that file previews now stay in the stable Files tab and display as an overlay.

Commit: 74d64c35dc5d1f2dbbde63836deecad0dcfe6e56

Verification:

  • git diff --check
  • Did not run full tests; this change only modifies documentation descriptions.

Additionally, before submitting, I tried pnpm build:check, but it was blocked by two existing unrelated broken docs links before entering pnpm test: docs/references/file/architecture.md:201 and docs/references/file/directory-tree.md:342.


Original Content

@zhangjiadi225 已更新 v2-refactor-temp/docs/chat/chat-layout-modes.md,移除了过期的 file-preview dynamic tab 描述,改为说明 file previews 现在保留在稳定的 Files tab 内以 overlay 展示。

Commit: 74d64c35dc5d1f2dbbde63836deecad0dcfe6e56

Verification:

  • git diff --check
  • 未跑全量测试;本次只改文档描述。

另外,提交前尝试过 pnpm build:check,但它在进入 pnpm test 前被两个既有无关 broken docs links 阻断:docs/references/file/architecture.md:201docs/references/file/directory-tree.md:342

@zhangjiadi225 zhangjiadi225 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The stale file-preview doc reference I raised has been updated to describe the stable Files tab overlay behavior. I also confirmed the latest head remains 74d64c35dc5d1f2dbbde63836deecad0dcfe6e56; core checks are passing, with render-test still in progress at review time.

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings:

  • No substantive issues found at the reviewed head. The previously raised overlay, watcher-gating, stale-selection, context-menu, Escape-focus, and docs issues appear addressed with relevant regression coverage.

Open questions:

  • None.

Summary:
Approving this re-review. Residual risk is limited to live Electron/UI behavior not runtime-verified in this unattended sweep.

Verification:

  • Reviewed PR #16713 at head 74d64c35dc5d1f2dbbde63836deecad0dcfe6e56 in /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260706T0828Z-16713-74d64c3 with three reviewer slices plus coordinator pass.
  • Checked GitHub CI: changes, catalog-hand-edit-check, changeset-check, basic-checks, render-test, and successful translate runs passed; general-test and claude skipped. One older duplicate translate run was cancelled but superseded by successful translate checks.
  • Ran git diff --check origin/main...HEAD.
  • Did not run local pnpm lint/test/format, per repository PR review rules.

@kangfenmao kangfenmao added the ready-to-merge This PR has sufficient reviewer approvals but is awaiting code owner approval. label Jul 6, 2026
@0xfullex 0xfullex removed the ready-to-merge This PR has sufficient reviewer approvals but is awaiting code owner approval. label Jul 6, 2026
@0xfullex

0xfullex commented Jul 6, 2026

Copy link
Copy Markdown
Member

@kangfenmao is requesting change

@kangfenmao kangfenmao added the ready-to-merge This PR has sufficient reviewer approvals but is awaiting code owner approval. label Jul 6, 2026
@0xfullex 0xfullex merged commit d9a5789 into main Jul 6, 2026
7 checks passed
@0xfullex 0xfullex deleted the feat/file-preview-overlay branch July 6, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has sufficient reviewer approvals but is awaiting code owner approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants